-
Notifications
You must be signed in to change notification settings - Fork 49
Abdm facility fetch changes #123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThe changes introduce a new REST controller, Changes
Poem
π Recent review detailsConfiguration used: CodeRabbit UI π Files selected for processing (1)
π§ Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? πͺ§ TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
π§Ή Outside diff range and nitpick comments (7)
src/main/java/com/iemr/common/service/abdmfacility/AbdmFacilityService.java (2)
5-5: Consider adding JavaDoc and reviewing the return type.The method signature is clear, but it could benefit from the following improvements:
- Add a JavaDoc comment to explain the method's purpose, parameter, and return value.
- Consider using a more specific return type if possible, such as an object representing the ABDM facility, instead of a String.
Example JavaDoc:
/** * Retrieves the mapped ABDM facility for a given provider service mapping ID. * * @param psmId The Provider Service Mapping ID * @return The mapped ABDM facility identifier or details */ public String getMappedAbdmFacility(int psmId);
1-7: LGTM: Well-structured service interface.The
AbdmFacilityServiceinterface is well-defined and follows good practices for service layer design. It provides a clear contract for implementing classes to handle ABDM facility mapping functionality.Minor suggestion: Consider adding an import statement for
java.lang.Stringat the beginning of the file for explicitness, although it's not strictly necessary asStringis automatically imported in Java.import java.lang.String;src/main/java/com/iemr/common/service/abdmfacility/AbdmFacilityServiceImpl.java (1)
1-22: Overall assessment: Good implementation with room for improvement.The
AbdmFacilityServiceImplclass provides the necessary functionality for retrieving mapped ABDM facility information. However, there are a few areas where the implementation can be enhanced:
- Consider using constructor injection instead of field injection for better testability and immutability.
- Refine the
getMappedAbdmFacilitymethod to handle potential null values and return more meaningful data.- Verify and correct the potential typo in the repository method name (
getAbdamFacilityvsgetAbdmFacility).- Add appropriate exception handling and logging throughout the service.
These improvements will make the service more robust, easier to test, and more maintainable in the long run.
src/main/java/com/iemr/common/repository/abdmfacility/AbdmFacilityRepository.java (1)
1-17: Consider adding documentation comments.To improve code readability and maintainability, consider adding Javadoc comments to the interface and the custom query method. This will help other developers understand the purpose and usage of this repository.
Here's an example of how you could add documentation:
/** * Repository interface for managing ABDM (Ayushman Bharat Digital Mission) facility data. * This interface provides CRUD operations for ProviderServiceAddressMapping entities * and includes a custom query for retrieving facility details. */ @Repository public interface AbdmFacilityRepository extends CrudRepository<ProviderServiceAddressMapping, Integer> { /** * Retrieves the ABDM facility details for a given Provider Service Address Mapping ID. * * @param pssmID The Provider Service Address Mapping ID * @return The ProviderServiceAddressMapping entity corresponding to the given ID */ @Query("SELECT v from ProviderServiceAddressMapping v where v.pSAddMapID = :pssmID") ProviderServiceAddressMapping getAbdmFacility(@Param("pssmID") int pssmID); }src/main/java/com/iemr/common/controller/abdmfacility/AbdmFacilityController.java (2)
16-17: Remove unused import.The
@RequestBodyannotation is imported but not used in the controller. Consider removing this unused import to keep the code clean.Apply this change:
-import io.swagger.v3.oas.annotations.parameters.RequestBody;
46-47: Improve logging for better traceability.The current log message doesn't include the
workLocationId, which could be useful for debugging. Consider adding more context to the log message.Enhance the log message as follows:
- logger.info("Get ABDM Registered facilities API response" + response.toString()); + logger.info("Get ABDM Registered facilities API response for workLocationId {}: {}", workLocationId, response);src/main/java/com/iemr/common/data/users/ProviderServiceAddressMapping.java (1)
84-91: LGTM! Consider adding JavaDoc comments for the new fields.The addition of
abdmFacilityIDandabdmFacilityNamefields is well-implemented and aligns with the PR objective. The use of@Exposeand@Columnannotations is correct and consistent with the existing code style.To improve code documentation, consider adding JavaDoc comments for these new fields. For example:
/** * The unique identifier for the ABDM facility. */ @Expose @Column(name = "AbdmFacilityID") private String abdmFacilityID; /** * The name of the ABDM facility. */ @Expose @Column(name = "AbdmFacilityName") private String abdmFacilityName;
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
π Files selected for processing (5)
- src/main/java/com/iemr/common/controller/abdmfacility/AbdmFacilityController.java (1 hunks)
- src/main/java/com/iemr/common/data/users/ProviderServiceAddressMapping.java (2 hunks)
- src/main/java/com/iemr/common/repository/abdmfacility/AbdmFacilityRepository.java (1 hunks)
- src/main/java/com/iemr/common/service/abdmfacility/AbdmFacilityService.java (1 hunks)
- src/main/java/com/iemr/common/service/abdmfacility/AbdmFacilityServiceImpl.java (1 hunks)
π§° Additional context used
π Additional comments (9)
src/main/java/com/iemr/common/service/abdmfacility/AbdmFacilityService.java (2)
1-1: LGTM: Package declaration is appropriate.The package name
com.iemr.common.service.abdmfacilityfollows Java naming conventions and correctly indicates its purpose within the service layer for ABDM facility-related functionality.
3-3: LGTM: Interface declaration is well-defined.The
AbdmFacilityServiceinterface is appropriately named and declared. It follows Java naming conventions and clearly indicates its purpose in the system.src/main/java/com/iemr/common/service/abdmfacility/AbdmFacilityServiceImpl.java (3)
1-8: LGTM: Package declaration and imports are correct.The package name is appropriate for the service implementation, and all necessary imports are present without any unused imports.
9-10: LGTM: Class declaration and annotation are correct.The class is properly annotated with @service, follows naming conventions, and implements the AbdmFacilityService interface.
15-20:β οΈ Potential issueRefine the implementation of getMappedAbdmFacility method.
There are a few points to consider in this method:
- There's a typo in the repository method name:
getAbdamFacilityshould likely begetAbdmFacility.- Returning
toString()directly might not provide the most useful or formatted output for a service method.- There's no null check before calling
toString(), which could lead to a NullPointerException.Here's a suggested improvement:
@Override public String getMappedAbdmFacility(int psmId) { - ProviderServiceAddressMapping res = abdmFacilityRepo.getAbdamFacility(psmId); - return res.toString(); + ProviderServiceAddressMapping res = abdmFacilityRepo.getAbdmFacility(psmId); + if (res == null) { + return null; // or throw a custom exception + } + // Return a specific field or a formatted string instead of toString() + return res.getAbdmFacilityId(); // Assuming this method exists }Also, consider adding appropriate exception handling and logging.
To verify the correct repository method name, please run:
src/main/java/com/iemr/common/repository/abdmfacility/AbdmFacilityRepository.java (2)
1-9: LGTM: Package declaration and imports are appropriate.The package name and imports are well-structured and contain all necessary components for the repository interface.
10-11: LGTM: Interface declaration is well-defined.The
AbdmFacilityRepositoryinterface is correctly declared, extendingCrudRepositoryforProviderServiceAddressMappingentities and properly annotated with@Repository.src/main/java/com/iemr/common/controller/abdmfacility/AbdmFacilityController.java (2)
19-26: Class structure and annotations look good.The controller is properly set up with the correct annotations, logger initialization, and dependency injection for the AbdmFacilityService.
31-31: Consider using the Authorization header.The
Authorizationheader is received as a parameter but not used in the method. If it's not needed, consider removing it. If it should be used for authentication, make sure to implement the necessary checks.To verify if the
Authorizationheader is used elsewhere in the service layer, run the following script:β Verification successful
Remove Unused Authorization Parameter
The
Authorizationheader is received as a parameter in thegetStoreStockDetailsmethod but is not utilized. If authentication is required, please implement the necessary checks; otherwise, consider removing the unused parameter.π Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of Authorization header in the service layer rg --type java -g 'src/main/java/com/iemr/common/service/**/*.java' 'Authorization'Length of output: 9603
src/main/java/com/iemr/common/service/abdmfacility/AbdmFacilityServiceImpl.java
Show resolved
Hide resolved
src/main/java/com/iemr/common/repository/abdmfacility/AbdmFacilityRepository.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iemr/common/controller/abdmfacility/AbdmFacilityController.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iemr/common/controller/abdmfacility/AbdmFacilityController.java
Show resolved
Hide resolved
|
@helenKaryamsetty several comments from coderabbit including typos and function names are not addressed. Have you checked these comments? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
π§Ή Outside diff range and nitpick comments (2)
src/main/java/com/iemr/common/controller/abdmfacility/AbdmFacilityController.java (2)
17-17: Remove unused import.The
@RequestBodyannotation import is not used in this GET endpoint and should be removed.-import io.swagger.v3.oas.annotations.parameters.RequestBody;
29-29: Enhance API documentation.The OpenAPI documentation could be more detailed. Consider adding:
- A more descriptive summary
- Response descriptions
- Parameter descriptions
-@Operation(summary = "Get Abdm facility mapped to worklocation") +@Operation( + summary = "Retrieve ABDM facility details for a work location", + description = "Fetches the mapped ABDM Facility ID and name for the given work location ID", + parameters = { + @Parameter(name = "workLocationId", description = "The ID of the work location to fetch ABDM facility details for"), + @Parameter(name = "Authorization", description = "Bearer token for authentication", in = ParameterIn.HEADER) + } +)
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
π Files selected for processing (1)
src/main/java/com/iemr/common/controller/abdmfacility/AbdmFacilityController.java(1 hunks)
π§° Additional context used
π Learnings (1)
src/main/java/com/iemr/common/controller/abdmfacility/AbdmFacilityController.java (1)
Learnt from: helenKaryamsetty
PR: PSMRI/Common-API#123
File: src/main/java/com/iemr/common/controller/abdmfacility/AbdmFacilityController.java:41-45
Timestamp: 2024-11-20T07:23:22.514Z
Learning: In the `Common-API` codebase, exception handling within controller classes like `AbdmFacilityController.java` is managed using the `OutputResponse` class, and it is not required to catch specific exceptions separately. General exception handling is sufficient in this context.
π Additional comments (1)
src/main/java/com/iemr/common/controller/abdmfacility/AbdmFacilityController.java (1)
19-26: LGTM!
The class setup follows Spring Boot best practices with proper annotations, logging, and dependency injection.
src/main/java/com/iemr/common/controller/abdmfacility/AbdmFacilityController.java
Show resolved
Hide resolved
src/main/java/com/iemr/common/controller/abdmfacility/AbdmFacilityController.java
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
yes committed those changes |
|
ravishanigarapu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks fine


π Description
JIRA ID: AMM-871
Changes to fetch mapped ABDM Facility Id from job location.
β Type of Change
βΉοΈ Additional Information
Please describe how the changes were tested, and include any relevant screenshots, logs, or other information that provides additional context.
Summary by CodeRabbit
New Features
Bug Fixes